Add include/exclude fields to package manifest#310
Conversation
asmello
force-pushed
the
asm/include-exclude
branch
from
57743f3 to
a010572
Compare
Introduce `include` and `exclude` fields on `PackageManifest`, allowing
users to control which files are packaged. The implementation uses the
`ignore` crate (`OverrideBuilder` for include, inverted match semantics
for exclude).
Key changes:
- Bump version to 0.14.0, add `ignore` dependency
- Add `include: Option<Vec<String>>` and `exclude: Vec<String>` to
`PackageManifest` with serde skip-if-empty semantics
- Add `Canary13` edition variant and accept it in the deserializer
- Extend `collect()` with `include` and `exclude` parameters, using
`OverrideBuilder` for include globs and inverted `filter_entry`
matching for exclude globs
- Wire include/exclude through `release()`, `populate()`, and `init()`
- Filter directory entries from `collect()` results to prevent panics
- Activate `TypesBuilder` proto filter with `select("proto")`
- Replace `.unwrap()` calls with proper error handling in `release()`
and `populate()`
- Fix `list()` to not apply root include/exclude to vendored files
- Fix `populated_files()` to not redundantly re-apply include/exclude
- Handle broken symlinks gracefully in exclude `filter_entry`
- Update all test fixtures and tutorial build.rs for new API
- Add unit tests for `collect()` and manifest deserialization
asmello
force-pushed
the
asm/include-exclude
branch
from
a010572 to
e7e3f4a
Compare
Align with Cargo's behavior: only one of `include` or `exclude` may be set on a package manifest. Add validation in manifest parsing, simplify the `collect()` logic now that the two cannot coexist, and update tests accordingly.
asmello
force-pushed
the
asm/include-exclude
branch
from
e7e3f4a to
fbcd143
Compare
- Reject empty `include = []` at parse time with a clear error message - Guard `release()` against missing `manifest.package` - Fix two raw.rs test fixtures that had both include and exclude set - Add test for empty include rejection - Add tests for populate() with include/exclude filtering - Document that Proto.toml is always included regardless of settings
|
Will have a look today. Before this, can you motivate the choice of |
| ) -> miette::Result<Vec<PathBuf>> { | ||
| debug_assert!( | ||
| include.is_none() || exclude.is_empty(), | ||
| "include and exclude are mutually exclusive" |
There was a problem hiding this comment.
In my naive understanding i would have assumed that we have:
Base = ./proto/**.proto
Include = <some-globs>
Exclude = <some-globs>
Final = (Base & Include) - Exclude
There was a problem hiding this comment.
Yeah, that's also possible (and in fact at first I implemented it that way). But that's not how Cargo works, and apparently the reason is that you never really need both, and there's more than one possible interpretation if both are provided (although I agree what you described is the most intuitive), so we avoid ambiguity and potential mistakes by disallowing this case. With only one list, you can directly infer the resulting filter, while with both it gets trickier to reason about what ends up being included.
| } | ||
|
|
||
| for entry in self.collect(&source_path, false).await { | ||
| let include = manifest.include.as_deref(); |
There was a problem hiding this comment.
Why is the include defined using let but the exclude one is passed as an arg directly?
There was a problem hiding this comment.
This is because exclude is Vec<_> while include is Option<Vec<_>> — we need to distinguish include not being set from it being set as an empty vec because those have different semantics. But now that I think about it, the same goes for exclude, because an empty exclude list actually includes more files than the default. Will update.
|
Overall i think this is a sane feature, and a non controversial implementation, but im not yet 100% sure on the semantics of the include and exclude fields. How is this implemented in prevalent package managers? I would go with the most common semantics here; if this is already the case, ignore my comments:) |
mara-schulke
added
component::cli
|
Also, can you in any case, please extend the book for this feature? |
We have a use case where we want to package an additional manifest file alongside the proto files. This additional metadata is related to the proto definitions, but inlining it as comments seems brittle and cumbersome. |
|
Entirely! I implemented the same semantics except that when omitted only |
`exclude: Vec<String>` collapsed an omitted field and `exclude = []` into the same empty vec, both falling through to the default `*.proto`-only collection path. Switch to `Option<Vec<String>>` so an explicit empty list takes the exclude branch (start from all files, remove nothing) — the only way to express "bundle every file" without listing every glob.
`include = []` produces a package containing only the manifest (always bundled by `Package::create`). That's a valid if unusual buffrs package, so there's no concrete reason to reject it at parse time. Drop the bail, swap the rejection test for a distinction-preserving one mirroring the empty-exclude case, and short-circuit in `collect()` so empty include actually yields no files — `OverrideBuilder` with no patterns is a no-op filter that would otherwise let everything through, the opposite of the intended "match nothing" semantic.
asmello
force-pushed
the
asm/include-exclude
branch
from
efb789b to
706ca6a
Compare
Pulls in the fix for RUSTSEC-2026-0104: a reachable panic when parsing certificate revocation lists with a syntactically valid empty BIT STRING in `IssuingDistributionPoint.onlySomeReasons`. Buffrs doesn't use CRLs directly, but the advisory still trips `cargo deny check` in CI.
The default-proto branch in `collect()` panicked via `.expect()` if `TypesBuilder` ever rejected the registration or build steps. Both are infallible in practice, but the rest of `collect()` already wraps errors as miette diagnostics, so propagate these too for consistent pretty printing. Addresses review comment on PR #310.
4 participants
Summary
includeandexcludefields toPackageManifest, allowing packages to control which files are included when packaging and populating vendor directoriesincludestarts from a blank slate — only files matching the provided globs are included (any file type, not just.proto)excludestarts from all files and removes those matching the provided globs.protofiles recursively)walkdirwith theignorecrate for glob matching, type filtering, and override support inPackageStore::collect()Canary13edition to snapshot current behavior before this changeTest plan
collect()with include patterns, exclude patterns, vendor filtering, file-only results, and invalid globscargo test)include/excludeinProto.tomland confirm correct files are packaged